Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deprecate some Pool methods related to groups and fix array shapes #6659

Merged
merged 4 commits into from
Dec 8, 2020
Merged

Deprecate some Pool methods related to groups and fix array shapes #6659

merged 4 commits into from
Dec 8, 2020

Conversation

franmomu
Copy link
Member

@franmomu franmomu commented Dec 6, 2020 •
edited
Loading

I was trying to manual merge 3.x, but phpstan failed because the array specified in GroupExtension is different from

SonataAdminBundle/src/Admin/Pool.php

Lines 108 to 111 in 8c0d58a

/**
* @return array<string, array<string, AdminInterface[]>>
*/
public function getDashboardGroups(): array

So I added more specific info.

Not sure if should be pedantic or if I should add a changelog.

While doing this I found that Pool::getGroups() method maybe it's not used because the code doesn't make sense with the current groups. #6659 (comment)

  • Check if Pool::getGroups() is used.

I am targeting this branch, because this is BC.

Changelog

### Deprecated
- Deprecated `Pool::getGroups()` method.
- Deprecated `Pool::hasGroup()` method.
- Deprecated `Pool::getAdminsByGroup()` method.

@OskarStark
Copy link
Member

Pedantic is fine IMO

core23
core23 reviewed Dec 6, 2020
View reviewed changes
src/Admin/Pool.php
@@ -136,9 +153,6 @@ public function __construct(
$this->propertyAccessor = $propertyAccessor;
}

/**
* @return array<string, array<string, AdminInterface>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the docs here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Psalm was failing and I'll check later today, but looks like this method does not work, even throws an error, I was going to check to deprecate it.

Based on this structure:

https://github.com/sonata-project/SonataAdminBundle/pull/6659/files#diff-96aac3a8c78cc9632a36cbb7489484b6c87ae336c1ceeaa028c4b265cafc6f38R41-R57

this method would try to to get and admin with id label, label_catalogue, icon, etc, that's why I guess it is not being used, but I haven't checked yet.

VincentLanglet
VincentLanglet reviewed Dec 6, 2020
View reviewed changes
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we deprecate getGroups, I think we can deprecate hasGroup

@franmomu
Deprecate Pool::getGroups
afde2f4 
This function does not work and it throws a RuntimeError because
the internal the array  property adminGroups changed some time
ago its content, but this function was not updated.
@franmomu
Copy link
Member Author

franmomu commented Dec 6, 2020 •
edited
Loading

So I've looked for uses of getGroups and pool.groups and I haven't found any. Using it, it throws a RuntimeException like An exception has been thrown during the rendering of a template ("Admin service "label" not found in admin pool. Did you mean "App\Admin\FooAdmin" or one of those: []?").

About the hasGroup() method and getAdminsByGroup(), also looks like they are not being used either, but they work fine I think, so should we deprecated them? I haven't used them, but I guess they are useful in some way: #1465.

@franmomu franmomu added minor and removed pedantic labels Dec 6, 2020
@VincentLanglet
Copy link
Member

About the hasGroup() method and getAdminsByGroup(), also looks like they are not being used either, but they work fine I think, so should we deprecated them? I haven't used them, but I guess they are useful in some way: #1465.

If they are not used, we should deprecate them. Less code is better.

@franmomu
Deprecate Pool::hasGroup and Pool::getAdminsByGroup
728f533 
They were added in sonata-project#1465,
but they have never been used in our code.
@franmomu franmomu changed the title Fix array shapes with groups Deprecate some Pool methods related to groups and fix array shapes Dec 7, 2020
@franmomu franmomu requested a review from a team December 7, 2020 13:00
@VincentLanglet VincentLanglet requested a review from a team December 7, 2020 16:04
@franmomu franmomu requested a review from core23 December 7, 2020 21:15
@VincentLanglet VincentLanglet merged commit 916877e into sonata-project:3.x Dec 8, 2020
@VincentLanglet
Copy link
Member

Thanks @franmomu

@franmomu franmomu deleted the fix_array_phpstan branch December 8, 2020 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VincentLanglet VincentLanglet VincentLanglet approved these changes

@core23 core23 core23 approved these changes

Assignees
No one assigned
Labels
minor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@franmomu @OskarStark @VincentLanglet @core23